Fix nine patch of circular TextureProgressBar #54345

floppyhammer · 2021-10-28T10:10:49Z

With nine patch enabled.

Before:

After:

kleonc · 2021-10-28T15:09:51Z

scene/gui/texture_progress_bar.cpp

@@ -450,48 +450,43 @@ void TextureProgressBar::_notification(int p_what) {
 							}

 							float val = get_as_ratio() * rad_max_degrees / 360;
-							if (val == 1) {
-								Rect2 region = Rect2(progress_offset, s);
-								Rect2 source = Rect2(Point2(), s);


I think just changing that line to:

Rect2 source = Rect2(Point2(), progress->get_size());

should fix the case when val == 1 which seemed to be the problem in here. And that case seems to be simpler/faster so I'd opt for keeping it in (instead of removing it like you did).

@kleonc Yes, you're right. Better keep it this way.

floppyhammer · 2021-10-29T08:53:10Z

@kleonc How about removing this block:

if (uvs.find(uv) >= 0) {
    continue;
}

Due to float precision, find() can't work properly here. Might cause some future issue.

kleonc · 2021-10-29T17:31:53Z

@kleonc How about removing this block:
if (uvs.find(uv) >= 0) {
    continue;
}
Due to float precision, find() can't work properly here. Might cause some future issue.

This block prevents adding twice exactly the same uv and a point generated for it:

godot/scene/gui/texture_progress_bar.cpp

Line 489 in d046817

points.push_back(progress_offset + Point2(uv.x * s.x, uv.y * s.y));

Due to float precision two similar uv values might possibly end up in the uvs but it should be fine as long as generated points form a valid polygon (so it could be rendered properly). And if generated polygon can currently indeed end up being invalid due to float precision then removing the block you've mentioned won't fix it anyway, it would only allow exactly the same uv/point pairs to occur.
I'm not sure if two exactly the same uv can be produced though. It could possibly occur only in case when start/end is almost equal to one of the corners and unit_val_to_uv(start)/unit_val_to_uv(end) would produce the same uv as unit_val_to_uv(that_corner). But again, not sure if this can happen. If it's impossible then the block you've mentioned could indeed be removed as that condition would never be met; otherwise it handles such situation.
So I'd just leave it as is. It doesn't do any harm besides adding a small overhead (uvs vector will have at most 7 elements).

Some notes after looking at the code (kinda out of the scope of this PR but I guess you could include them in here if you want):

pts shouldn't be an Array as it stores Variants. Probaly Vector<float> or LocalVector<float> should be used (edit: or just float[6] + count of the elements stored, see this comment on RC).

godot/scene/gui/texture_progress_bar.cpp

Line 458 in d046817

Array pts;
You can easily get rid of pts.sort() by making corners be sorted in the first place and adding from/to before/after the corners:

diff --git a/scene/gui/texture_progress_bar.cpp b/scene/gui/texture_progress_bar.cpp
index 35a098c6fd..17b888f759 100644
--- a/scene/gui/texture_progress_bar.cpp
+++ b/scene/gui/texture_progress_bar.cpp
@@ -379,7 +379,7 @@ void TextureProgressBar::draw_nine_patch_stretched(const Ref<Texture2D> &p_textu
 }
 
 void TextureProgressBar::_notification(int p_what) {
-	const float corners[12] = { -0.125, -0.375, -0.625, -0.875, 0.125, 0.375, 0.625, 0.875, 1.125, 1.375, 1.625, 1.875 };
+	const float corners[12] = { -0.875, -0.625, -0.375, -0.125, 0.125, 0.375, 0.625, 0.875, 1.125, 1.375, 1.625, 1.875 };
 	switch (p_what) {
 		case NOTIFICATION_DRAW: {
 			if (nine_patch_stretch && (mode == FILL_LEFT_TO_RIGHT || mode == FILL_RIGHT_TO_LEFT || mode == FILL_TOP_TO_BOTTOM || mode == FILL_BOTTOM_TO_TOP || mode == FILL_BILINEAR_LEFT_AND_RIGHT || mode == FILL_BILINEAR_TOP_AND_BOTTOM)) {
@@ -458,16 +458,15 @@ void TextureProgressBar::_notification(int p_what) {
 								}
 
 								float end = start + direction * val;
-								pts.append(start);
-								pts.append(end);
 								float from = MIN(start, end);
 								float to = MAX(start, end);
+								pts.append(from);
 								for (int i = 0; i < 12; i++) {
 									if (corners[i] > from && corners[i] < to) {
 										pts.append(corners[i]);
 									}
 								}
-								pts.sort();
+								pts.append(to);
 								Vector<Point2> uvs;
 								Vector<Point2> points;
 								uvs.push_back(get_relative_center());

Also corners could be just removed and:

const float corners[12] = { -0.875, -0.625, -0.375, -0.125, 0.125, 0.375, 0.625, 0.875, 1.125, 1.375, 1.625, 1.875 };
...
pts.append(from);
for (int i = 0; i < 12; i++) {
	if (corners[i] > from && corners[i] < to) {
		pts.append(corners[i]);
	}
}
pts.append(to);

could be changed to:

pts.append(from);
for (float corner = Math::floor(from * 4 + 0.5) * 0.25 + 0.125; corner < to; corner += 0.25) {
	pts.append(corner);
}
pts.append(to);

floppyhammer · 2021-11-01T06:16:58Z

@kleonc Good idea.

kleonc

pts are still Array but it's unrelated to the bugfix in here so it can be changed in another PR.
I've checked again math in the corner calculations (which are also unrelated to the bugfix) and it looks correct.
Should be fine to merge.

akien-mga · 2021-11-26T07:24:02Z

Thanks!

akien-mga · 2021-11-29T15:11:45Z

Cherry-picked for 3.5.

Calinou · 2022-01-19T23:13:33Z

@akien-mga Should this be cherry-picked to 3.4.3?

akien-mga · 2022-01-25T17:48:03Z

Cherry-picked for 3.4.3.

floppyhammer requested a review from a team as a code owner October 28, 2021 10:10

akien-mga added bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:gui labels Oct 28, 2021

akien-mga added this to the 4.0 milestone Oct 28, 2021

kleonc reviewed Oct 28, 2021

View reviewed changes

Fix nine patch of circular TextureProgressBar

f9b1ce0

floppyhammer force-pushed the fix-circular-texture-progress branch from 5f37415 to f9b1ce0 Compare November 1, 2021 06:15

kleonc approved these changes Nov 1, 2021

View reviewed changes

KoBeWi approved these changes Nov 26, 2021

View reviewed changes

akien-mga merged commit 3a0a935 into godotengine:master Nov 26, 2021

floppyhammer deleted the fix-circular-texture-progress branch November 26, 2021 08:21

akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Nov 29, 2021

raulsntos mentioned this pull request Jan 19, 2022

texture progress progress texture becomes empty when the value is 100 #56975

Closed

akien-mga added the cherrypick:3.4 label Jan 20, 2022

akien-mga removed the cherrypick:3.4 label Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nine patch of circular TextureProgressBar #54345

Fix nine patch of circular TextureProgressBar #54345

floppyhammer commented Oct 28, 2021

kleonc Oct 28, 2021

floppyhammer Oct 29, 2021

floppyhammer commented Oct 29, 2021

kleonc commented Oct 29, 2021 •

edited

Loading

floppyhammer commented Nov 1, 2021

kleonc left a comment

akien-mga commented Nov 26, 2021

akien-mga commented Nov 29, 2021

Calinou commented Jan 19, 2022

akien-mga commented Jan 25, 2022

Fix nine patch of circular TextureProgressBar #54345

Fix nine patch of circular TextureProgressBar #54345

Conversation

floppyhammer commented Oct 28, 2021

kleonc Oct 28, 2021

Choose a reason for hiding this comment

floppyhammer Oct 29, 2021

Choose a reason for hiding this comment

floppyhammer commented Oct 29, 2021

kleonc commented Oct 29, 2021 • edited Loading

floppyhammer commented Nov 1, 2021

kleonc left a comment

Choose a reason for hiding this comment

akien-mga commented Nov 26, 2021

akien-mga commented Nov 29, 2021

Calinou commented Jan 19, 2022

akien-mga commented Jan 25, 2022

kleonc commented Oct 29, 2021 •

edited

Loading